Keep plan review cards in document flow - #249
Conversation
johannesjo
left a comment
johannesjo
left a comment
There was a problem hiding this comment.
I found two behavior regressions in the new flow-slot interaction, both reproduced in Chromium against the DOM structure introduced here:
- A multi-block selection that crosses an existing flow slot includes the card review/answer text in
Selection.toString(), so subsequent review or Ask prompts can quote UI content as plan content. - Highlight geometry is a one-time snapshot even though preceding slots now reflow, resize, and disappear, so the persisted highlight can drift away from the selected text.
The added tests only assert source strings and CSS, so they do not exercise either DOM behavior.
There is also a current integration blocker: main advanced to 1aabea4 and the PR now conflicts in PlanViewerDialog.tsx around the scroll-target guard (if (!target?.id) return). Please update the branch and preserve that guard while resolving the conflict.
| function handleMouseUp(event: MouseEvent) { | ||
| if (!contentRef) return; | ||
| const eventTarget = event.target; | ||
| if (eventTarget instanceof Element && eventTarget.closest(PLAN_REVIEW_FLOW_SLOT_SELECTOR)) { |
There was a problem hiding this comment.
This only rejects a selection whose mouse-up endpoint is inside a slot. Because the portal is physically mounted inside contentRef, a range that crosses an existing slot but ends on later plan text still passes this guard. I reproduced that range in Chromium: Selection.toString() included Review · Goal Previous feedback between the two plan paragraphs. getPlanSelection then stores that UI text as selectedText, polluting the next review or Ask prompt. Please exclude [data-plan-review-flow-slot] descendants from selection extraction/highlighting and add a DOM-level regression test for a range spanning an existing card.
| const { y, rects } = captureSelectionGeometry(); | ||
| setSelectionY(y); | ||
| setHighlightRects(rects); | ||
| setHighlightRects(captureSelectionGeometry()); |
There was a problem hiding this comment.
These rectangles are captured before the previous pending slot is removed on the next line. If that slot is above the new selection, its removal immediately shifts the selected text while the absolute overlay keeps the old coordinates; an 80px test slot produced 96px of drift in Chromium. An earlier Ask card expanding while streaming, or a sidebar dismissal, causes the same stale geometry. Please make the highlight track layout changes (for example by retaining the range and recalculating), and at minimum remove the prior pending slot before capturing the new geometry.
1409bbc to
65fb8d7
Compare
|
Addressed all requested changes in
Validation: full test suite 1,755 passed/23 skipped plus 11 client tests; type checks, static checks, and frontend build pass. |
2 participants
Summary
Addresses the plan-review inline comment overlap item in #214.
Validation
npm test -- src/components/plan-review-flow.test.ts(3 passed)npx vitest run src(65 files passed, 792 tests passed)npm run checknpm run check:staticnpm run build:frontend